added the reconnect timeout#2026
Conversation
|
@samhed, please check this PR |
samhed
left a comment
samhed
left a comment
There was a problem hiding this comment.
Thanks for your contribution. I didn't test it yet, but a few comments from briefly checking the code.
samhed
left a comment
samhed
left a comment
There was a problem hiding this comment.
I tested this and unfortunately, this breaks the reconnection functionality. No matter what I set reconnect_max_time to, the first time reconnect() is called, we abort the reconnection attempts. This is because UI.firstReconnectTime is never set.
I get the impression that you're not testing your code?
| {} | ||
| { | ||
| "reconnect_max_time": 600000 | ||
| } |
There was a problem hiding this comment.
The idea is that this should be empty. You will still get the same behavior since you added a default when calling UI.getSetting().
defaults.json could be used by integrators of the noVNC app to provide other defaults.
| if (UI.firstReconnectTime === null) { | ||
| UI.firstReconnectTime = Date.now(); | ||
| } | ||
| const elapsedTime = Date.now() - UI.firstReconnectTime; |
There was a problem hiding this comment.
It seems you're not using this variable.
| UI.closePowerPanel(); | ||
| } | ||
| }, | ||
| resetFirstReconnection(){ |
| } | ||
| UI.showStatus(msg); | ||
| UI.updateVisualState('connected'); | ||
| // Here we can reset the retry count |
| const maxTime = UI.getSetting('reconnect_max_time') ?? 600000; // default to 10 minutes => 10 * 60 * 1000 ms | ||
|
|
||
| // Initialize first reconnect time if it's the first attempt | ||
| if (UI.firstReconnectTime === null) { |
There was a problem hiding this comment.
UI.firstReconnectTime is never null, meaning your logic never works. Did you mean to check for 0?
|
|
||
| reconnect() { | ||
| UI.reconnectCallback = null; | ||
| const maxTime = UI.getSetting('reconnect_max_time') ?? 600000; // default to 10 minutes => 10 * 60 * 1000 ms |
There was a problem hiding this comment.
Perhaps this should be configured to seconds instead of milliseconds?
I.e:
const maxTime = UI.getSetting('reconnect_max_time') ?? 600; // default to 10 minutes => 10 * 60 seconds
...
// Check if we've exceeded the max reconnect time
if ((Date.now() - UI.firstReconnectTime) >= maxTime * 1000) {
2 participants
This update introduces a maximum reconnect timeout for the VNC client. Currently, when the VNC client attempts to reconnect, it may continue retrying indefinitely if the connection keeps failing, which is not ideal.
With this change, we add a new configuration key, reconnect_max_time, in default.json. This setting allows us to define a maximum duration for reconnection attempts. Once the specified time limit is reached, the client will stop trying to reconnect, preventing unnecessary retries and improving overall stability.